Skip to content

fix(wr-agent): a resolve answers at a deadline, so a hung mount can't hold a read slot (#334) - #346

Merged
joelmoss merged 8 commits into
masterfrom
fix/334-resolve-deadline
Oct 4, 2026
Merged

joelmoss merged 8 commits into
masterfrom
fix/334-resolve-deadline

Conversation

@joelmoss

@joelmoss joelmoss commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #334.

A remote cmd-click on link/../file.rb makes the agent's File service resolve the path on the host (#327). If the link leads onto a hung NFS or FUSE mount, canonicalize blocks for as long as the mount does. The request thread held one of the four read slots and a permit the whole time, so four clicks left every file read on the host Busy until the agent restarted.

wr-agent

  • The deadline. resolve runs its walk on its own thread (with_deadline) and answers after 10 seconds whether or not the walk has returned. The answer is an Io timeout with a fixed prefix, RESOLVE_TIMED_OUT. Its request thread holds its read slot and permit until the reply is written, and a hung walk holds neither, so reads and idle exit carry on.
  • The cap. Walks left behind are counted exactly once in ABANDONED_WALKS, using a per-walk state word, and a panic in the walk still updates the count. Once four are counted, resolve answers Busy without starting another. Up to seven can be parked, since walks already waiting can still be left behind.
  • Hand-off. hand_off is refused while any walk is left behind. A FUSE request the daemon has already read waits uninterruptibly (request_wait_answer in Linux's fs/fuse/dev.c), and execve waits for every other thread to die, so the hand-off would freeze every session on the host. That keeps the behaviour the held permit used to give. The check is race-free: with Quiet held, no request starts.
  • Thread start. The walk thread starts through thread::Builder, so if the OS refuses a thread the answer is Busy instead of a panic.
  • Logging. The agent logs each walk it leaves behind and each one that returns.

macapp

  • A click stops at the first resolve that times out (AgentFileProvider.resolveTimedOut). Its other candidates resolve the same prefix, or would read through the same link with no deadline, so nothing more in that click reaches the host. One click on a dead mount therefore costs 10 seconds and one walk, instead of up to 40 seconds and four walks.

Docs: docs/designs/remote-workrooms.md "Path handling" and the hand-off section cover:

Follow-ups filed (all part of #154):

Accepted trade-offs from the adversarial review:

  • The abandoned-walk count is per agent, so one workroom's dead link affects every workroom on that host.
  • The hand-off refusal also covers mounts whose waits are killable, such as NFS hard mounts, because userspace can't tell those apart from FUSE.

Test Coverage

CODE PATHS (vcs/crates/wr-agent/src/file.rs, handoff.rs)
- resolve: invalid relative path rejected before any I/O        [★★ TESTED]
- resolve: Busy at MAX_ABANDONED_WALKS                          [★★ TESTED] a_resolve_is_refused_busy_while_too_many_earlier_walks_are_still_out
- with_deadline: answers in time                                [★★ TESTED]
- with_deadline: timeout, walk left behind, Io "resolving timed out", count +1   [★★★ TESTED]
- with_deadline: a late walk takes the count back once          [★★★ TESTED]
- walks_left_behind() reflects the count                        [★★ TESTED]
- catch_unwind: panic before the deadline -> Io "panicked"      [★★★ TESTED]
- catch_unwind: panic after the deadline still leaves the count [★★★ TESTED]
- timeout branch "finished in the meantime" race arm            [GAP] intentionally untested: needs a production hook
- Disconnected arm                                              [GAP] intentionally untested: unreachable under catch_unwind
- Builder::spawn failure -> Busy                                [GAP] intentionally untested: the OS refusing a thread can't be forced
- hand_off refuses while walks_left_behind() > 0                [GAP] intentionally untested: needs a live serve context and a hung mount; checked by reading

CODE PATHS (macapp)
- remoteFile: a resolve timeout ends the click                  [★★★ TESTED] testARemoteParentTraversalIsResolvedOnTheHost (mutation-checked)
  • Coverage gate: 67% value-weighted. The 4 paths above are marked intentionally untested by the owner, each with its reason.
  • Test value: 2 tests written, 4 rejected by the authoring gate, 1 existing test extended, 0 paths weakly covered. The review rounds added the deadline, cap and stop-rule tests.
  • Every new test was checked red against a mutation of the code it covers:
    • the cap check removed;
    • the abandoned walk not counted;
    • with_deadline waiting for the walk (fails in 7 s, it doesn't hang);
    • catch_unwind removed;
    • walks_left_behind() returning 0;
    • the stop rule disabled.
  • The Rust deadline test asserts the literal "resolving timed out" that the app matches.
  • Base control: unavailable (no package.json ecosystem).

Pre-Landing Review

Before /ship: the independent adversarial pass that macapp/AGENTS.md rule 3 requires for any wr-agent change found five issues, all fixed. The serious one: a hand-off could execve behind an uninterruptible FUSE walk and freeze every session. The other four:

  • the cap was overstated;
  • a thread that failed to start would panic;
  • one test would have hung instead of failing;
  • the read-cap test could have passed for the wrong reason.

/ship review: this ran past the skill's 3-round fix limit, with the owner approving each extra round. The review log records cycles: 5, converged: false.

  • Round 1 (specialists + Red Team; performance and security found nothing):
    • a click stops at the first resolve timeout. Without this, one click on a dead mount could cost up to 40 s and four walks;
    • comments now say slots are held until the reply is written;
    • the hand-off docs list the new refusal and its effect on a remote host;
    • kept, at the owner's call: counting abandoned walks rather than live ones (simplification advisory).
  • Rounds 2–3: comment and doc corrections only.
  • After Step 11: the click-stop rule was narrowed to "skip only the same prefix", following Codex's P2. The lean re-review found that narrowing let a candidate with no .. read through the hung link with no deadline, so it was reverted to "a timeout ends the click" at the owner's call. The lost-click case is filed as A remote cmd-click whose :line comes before its .. loses its healthy variant when the resolve times out #345.
  • Review binding: the final tree differs from the last fully reviewed tree (bdaf2a3c) only by the log lines, which the lean re-review covered, and by comment, doc and TODO wording.

Step 11 adversarial:

Exploratory QA

Functional surface: the wr-agent File service resolve.

  • Probe 1: the resolve tests pass 6/6.
  • Probe 2: the deadline, cap and panic edge cases pass 19/19.
  • Verdict: PASS.
  • Limit: no real hung NFS or FUSE mount was available. The deadline is driven with a blocked walk through with_deadline, and the hand-off refusal was checked by reading the code.

Design Review

No frontend files changed — design review skipped.

Eval Results

No prompt-related files changed — evals skipped.

Scope Drift

Scope Check: CLEAN. Beyond #334:

  • the app-side stop rule, which the review asked for;
  • a P0 TODO for a relay test that flaked under load and is unrelated to this branch.

Plan Completion

No plan file detected. The plan was #334's "How to fix", the request, and the independent review's findings: 12 items, 9 DONE, 1 CHANGED. The CHANGED item: it detaches the walk (#334's first option) rather than walking components one at a time with openat. 2 items the diff can't show were confirmed by evidence:

  • the FUSE kernel behaviour, from fs/fuse/dev.c fetched from torvalds/linux;
  • the independent review, from its report, with its findings in the diff.

Verification Results

No plan-specific verification items. Automatic diff-scoped QA is above.

TODOS

No TODO items completed in this PR. Added one P0: a_relay_with_no_agent_fails_fast_and_says_so flaked once while the full Swift suite ran alongside it. It passed 5/5 alone, and this branch doesn't touch the relay.

Documentation

Status: current. No edits needed.

Scope: committed diff 4862781..b44aa65, with nothing staged, unstaged or untracked. Docs audited: README.md, CONTRIBUTING.md, AGENTS.md, macapp/AGENTS.md, macapp/README.md, docs/repository-notes.md, docs/designs/remote-workrooms.md and TODOS.md (read only). docs/agents, DESIGN.md, PRODUCT.md, the other docs/designs files and .agents/skills were searched for File service, hand-off refusal and remote cmd-click content, and none was affected.

Documentation health:

Coverage debt: #334 has reference and explanation coverage in remote-workrooms.md. There is no how-to or tutorial page, which is acceptable for an internal agent behaviour. No gaps were filled. No architecture diagrams are affected.

Test plan

  • cargo test --manifest-path vcs/Cargo.toml --workspace: all suites pass on b44aa653 (run alone; see TODOS for the relay flake under parallel load)
  • make app-test: full Swift unit suite passes on b44aa653
  • cargo fmt --check, cargo clippy -D warnings, make app-lint: clean

🤖 Generated with Claude Code

joelmoss and others added 7 commits October 4, 2026 16:25
… hold a read slot (#334)

A `resolve` walks through whatever a committed link points at, and on a hung
network mount that walk can block in the kernel for as long as the mount
does. Its request thread held a read slot and a permit the whole time, so
four clicks left every read Busy until the agent restarted.

The walk now runs on its own thread, and `resolve` answers after 10 seconds
whether or not it has returned. The request thread gives back its read slot
and permit when it answers; a walk left behind holds neither, so reads and
idle exit carry on. Once four walks are counted as left behind, `resolve`
answers Busy without starting another; up to seven can be parked, since walks
already waiting can still be left behind.

A hand-off is refused while any walk is left behind. A kill doesn't always
end one: a FUSE request the daemon has already read waits uninterruptibly
(`request_wait_answer` in Linux's fs/fuse/dev.c), and a hand-off's execve
waits for every other thread to die, so it would freeze every session on the
host. That keeps the hand-off behaviour the held permit used to give.

The walk thread is started with `thread::Builder`, so a thread the OS refuses
answers Busy instead of leaving the app to wait out its own timeout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A click stops at the first resolve that times out: every candidate of the
  same word resolves the same prefix through its last `..`, so each would
  hang another 10 s and leave another walk behind. The agent's timeout text
  is a constant, pinned by a test, that the app matches.
- Tests for a walk that panics, before and after its deadline, and for
  `walks_left_behind()`.
- Comments say a resolve holds its slot and permit until its reply is
  written, as a read does; the deadline only stops a hung walk stretching it.
- The design doc lists a stuck file lookup among the hand-off refusals and
  says what a remote host shows while one is stuck.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The with_deadline doc and the design doc now say a resolve timeout ends the
click, that a mount slow to wake can take a second click, and that only a
remote host meets a stuck file lookup at hand-off.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fix (#334)

From the adversarial review: a click stopped at the first resolve that timed
out, assuming every candidate of a word resolves the same prefix. A `:12`
before the `..` breaks that (`good.rb:12/link/../t.rb` also gives plain
`good.rb`), so a hung link could suppress a healthy file. The click now
skips only candidates through the same prefix as one that timed out.

The agent logs each walk it leaves behind and each one that returns, and
the design doc says a restart can't clear a walk parked in a FUSE wait.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The re-review of the previous commit found that skipping only candidates
through the same prefix lets a later candidate with no `..` read through
the same hung link (`sub/link` from `sub/link/..`), and `read` has no
deadline (#343), so one click could hold a read slot forever. Back to
ending the click at the first resolve that times out: nothing more in that
click reaches the host. The contrived word whose `:line` sits before its
`..` loses its healthy variant again; tracked as #345, to revisit once
reads have a deadline.

The log lines and the note that a restart can't clear a FUSE-parked walk
stay.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T16:44:44.530986Z bed7713 New commits
🔒 Security Review ✅ Completed 2026-10-04T16:45:57.505725Z bed7713 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Hand-off safety needs human validation against hung mounts, and the resolve-cap fallback still permits an unbounded read.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Addresses #334 by bounding remote path resolution so hung mounts cannot indefinitely retain resolve read slots.

Changes:

  • Adds a 10-second resolve deadline, abandoned-walk accounting, and hand-off protection.
  • Stops remote cmd-click probing after a resolve timeout.
  • Adds regression coverage and documents operational limits.
File Description
vcs/​crates/​wr-agent/​src/​handoff.rs Refuses hand-off while abandoned walks remain.
vcs/​crates/​wr-agent/​src/​file.rs Adds deadlines, walk limits, logging, and tests.
TODOS.md Records an unrelated relay-test flake.
macapp/​WorkroomAppTests/​TerminalLinkOpenerTests.swift Tests timeout-driven click termination.
macapp/​WorkroomApp/​Core/​TerminalSessions.swift Stops candidate probing after resolve timeouts.
macapp/​WorkroomApp/​Core/​AgentFileProvider.swift Defines the timeout-message prefix.
docs/​designs/​remote-workrooms.md Documents deadlines, hand-off refusal, and limitations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +2273 to 2280
} catch FileServiceError.failed(let message)
where message.hasPrefix(AgentFileProvider.resolveTimedOut)
{
// The walk hung on a mount (#334). The other candidates differ only after the last
// `..`, so they would each hang another 10 s and leave another walk behind.
NSLog("Workroom: resolving %@ on the host timed out", onHost)
return nil
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Left by Claude Code.

Fixed in bed7713. The cap refusal (too many earlier resolves are still walking) now ends the click as a timeout does: TerminalSessions.remoteFile matches AgentFileProvider.resolveWalksBusy alongside resolveTimedOut and returns before any later candidate is probed.

  • The agent's cap test now asserts that exact text, so the two sides can't drift apart.
  • A new case in testARemoteParentTraversalIsResolvedOnTheHost covers link/file.rb:12/../t.rb. The literal candidate gets the cap refusal, and link/file.rb exists, so a fallback read would return it. The test asserts nil, and it went red with the new match removed.

From Copilot's review of #346: once four walks are stuck, `resolve` answers
Busy("too many earlier resolves are still walking"), which missed the
timeout-only catch. The click then went on to the next candidate, and for a
word like `link/file.rb:12/../t.rb` that is plain `link/file.rb`, a read
through the same hung link with no deadline (#343). The app now ends the
click on that refusal as on a timeout. Its text is pinned by the cap test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@joelmoss
joelmoss merged commit 18b3b0d into master Oct 4, 2026
22 of 23 checks passed
@joelmoss
joelmoss deleted the fix/334-resolve-deadline branch October 4, 2026 17:06
@joelmoss joelmoss mentioned this pull request Oct 4, 2026
20 of 21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wr-agent resolve through a link onto a dead mount holds a File read slot until the agent restarts

2 participants